Skip to content

fix(cron): clean up output dir when auto-removing repeat-limited job - #22066

Open
nftpoetrist wants to merge 1 commit into
NousResearch:mainfrom
nftpoetrist:fix/cron-cleanup-output-on-repeat-limit
Open

fix(cron): clean up output dir when auto-removing repeat-limited job#22066
nftpoetrist wants to merge 1 commit into
NousResearch:mainfrom
nftpoetrist:fix/cron-cleanup-output-on-repeat-limit

Conversation

@nftpoetrist

@nftpoetrist nftpoetrist commented May 8, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

mark_job_run() removes a job from jobs.json when its repeat limit is reached, but the corresponding OUTPUT_DIR/<job_id> directory (written by save_job_output() on every run) was never deleted. Each completed one-shot job leaves behind an orphaned output directory that accumulates indefinitely.

remove_job() gained the identical cleanup in PR #21882 (merged today). This PR extends that fix to mark_job_run(), the only other code path that deletes a job — parity with remove_job() lines 700–703.

One-line fix in mark_job_run(), scoped to the repeat-limit branch. No behavior change for recurring jobs or jobs without a repeat limit.

Related Issue

Fixes #22065

Type of Change

  • 🐛 Bug fix
  • ✨ New feature
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests
  • ♻️ Refactor
  • 🎯 New skill

Changes Made

  • cron/jobs.py: add shutil.rmtree(OUTPUT_DIR / job_id) guard in the repeat-limit auto-delete branch of mark_job_run() (+4 lines)
  • tests/cron/test_jobs.py: two new tests in TestMarkJobRun (+33 lines)
    • test_repeat_limit_cleans_up_output_dir — verifies the dir is removed after auto-delete
    • test_repeat_limit_no_output_dir_is_safe — verifies no crash when no output was saved

How to Test

python3.11 -m pytest tests/cron/test_jobs.py::TestMarkJobRun -v --override-ini="addopts="

All 12 tests pass (1 skip for missing croniter — pre-existing on macOS).

Checklist

Code

  • Contributing Guide read
  • Conventional Commits
  • No duplicate PR
  • This fix only
  • pytest run
  • Tests added
  • Platform: macOS

Documentation & Housekeeping

  • Docs updated — N/A
  • cli-config.yaml.example — N/A
  • CONTRIBUTING.md/AGENTS.md — N/A
  • Cross-platform impact — N/A (shutil.rmtree is cross-platform)
  • Tool descriptions — N/A

@nftpoetrist
fix(cron): clean up output dir when auto-removing repeat-limited job
ad50928 
mark_job_run() pops a job from jobs.json when its repeat limit is reached
(lines 737-741) but did not delete OUTPUT_DIR/<job_id>. save_job_output()
creates that directory on every run, so completed one-shot jobs leave one
orphaned output directory per job. remove_job() gained the identical cleanup
in PR NousResearch#21882 — this extends the fix to the auto-delete path inside
mark_job_run(), which is the only other code path that removes a job.

Fix: add the same OUTPUT_DIR/<job_id> rmtree guard used by remove_job(),
scoped to the repeat-limit branch. No behavior change for recurring jobs
or jobs without a repeat limit.

Fixes NousResearch#22065
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management labels May 9, 2026
@alt-glitch alt-glitch mentioned this pull request May 11, 2026
Open
13 tasks
@teknium1

teknium1 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks for identifying the orphaned-output path. The premise is still valid on current main: cron/scheduler.py:3427 saves output before cron/scheduler.py:3484 calls mark_job_run(), while the repeat-limit branch at cron/jobs.py:1426-1430 removes only the job record.

Problems

  • The proposed OUTPUT_DIR / job_id path bypasses current path validation. cron/jobs.py:255-268 defines _job_output_dir() to reject escape attempts, and remove_job() resolves it before saving at cron/jobs.py:1363-1370. Use that same ordering in the repeat-limit branch.
  • Current main has two additional exhausted-one-shot removal paths: claim_dispatch() at cron/jobs.py:1501-1506 and _get_due_jobs_locked() at cron/jobs.py:1818-1830. Both can leave output behind, so fixing only mark_job_run() does not cover the current bug class.

Suggested changes

  • Resolve the safe output path before removing/persisting each job, then delete it after the store update.
  • Add seeded-output regression tests for the repeat-limit, dispatch-limit, and stale-due removal paths.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@GottZ GottZ mentioned this pull request Jul 29, 2026
Open

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix(cron): mark_job_run() auto-delete leaves orphaned output dirs when repeat limit is reached

3 participants

@nftpoetrist @teknium1 @alt-glitch